feat(runtime): guide Auto tool selection by final tool surface - #3705
feat(runtime): guide Auto tool selection by final tool surface#3705testikun wants to merge 2 commits into
Conversation
Generated-by: Codex
18a5aba to
191282e
Compare
yunaremaia
left a comment
There was a problem hiding this comment.
Nice, clean slice — the eligibility matrix is well chosen (bypass needs no anti-bypass guidance, explore has no mutation tools to name), the fragment is deterministic and pure, and the tests cover both the positive path and every exclusion branch including the partial-tool-surface case. The bump of INTERACTIVE_RUN_COMPOSER_REVISION for the prompt-shape change is also the right call.
One coupling worth making explicit while this is fresh:
The Bash gate lives two checks away from its supporting fact. In interactive-run-composer.ts you compute
shellAvailable: input.shell?.setupError === undefined,which maps an absent shell to undefined rather than false. The resolver then only treats literal false as disqualifying, so a session with no shell at all still passes the shellAvailable check — it survives solely because a missing shell also removes 'Bash' from tools, and resolveAutoToolGuidance independently requires toolNames.includes('Bash'). That's correct today, but the safety of the first check depends on an invariant maintained by a different subsystem (whatever guarantees "no shell ⇒ no Bash tool entry"). If that ever drifts — say a future surface exposes a remote-shell Bash entry without a local input.shell — the guidance would advertise shell-first workflows into a session that cannot run them, and nothing here would fail loudly.
Two cheap ways to pin it down; either works:
- Treat absence as absence: pass
shellAvailable: input.shell !== undefined && input.shell.setupError === undefinedand keep the resolver's=== falsecheck (or switch both to truthiness). One expression, no cross-module assumption. - Keep the current wiring but add one line to the resolver doc comment stating that
shellAvailable === undefinedmeans "unknown" and eligibility then rests entirely on thetoolNamesgate — so the next reader knows the pairing is intentional.
Not blocking — behavior is correct as written — but I'd sleep better with option 1.
Generated-by: Codex
|
Thanks @yunaremaia — fixed in commit bf96cd0 using option 1. An absent shell plan is now explicitly treated as unavailable by requiring both a defined shell plan and no setup error, so Auto guidance no longer relies on the separate final-tool-surface gate to stay safe. I also updated the positive prompt fixtures to include a valid shell plan and added a regression case proving that a Bash-shaped tool surface without a shell plan does not receive the guidance. Verification: Runtime Auto guidance 4/4 passing; Runtime Host 1137/1137 passing; Biome and git diff --check clean. |
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no blocking issues.
The fix adds Auto-mode tool guidance as a pure function that chooses whether to inject a deterministic prompt based on the final tool surface and Host permission snapshot. It is minimal (1 file + 1 call site), correctly handles restricted surfaces and permission bypass cases, and bumps the composer revision to invalidate caches.
Checks on bf96cd058c are test: pending — code side is GO. Minor note: this adds a small new concept/module (+72 lines); the increment is valuable for recovery (#3507) and well isolated, so no action required.
简体中文
该头未发现阻断问题,轻度增熵有价值。|
Holding review here — the premise question is on #3507. Implementation quality isn't the issue: 443 lines across 5 files, pure resolver, well-gated eligibility, no design doc in the PR. Nicely contained. But one thing is visible from the diff and worth raising now: the fragment doesn't do what the issue asks for. #3507 quotes a directive instruction — do the work through Bash wherever it can, fall back to dedicated tools only when Bash genuinely can't. What So both paths lead to the same place: if the retry-loop problem is real, this text probably doesn't move it; if it isn't real, we've added permanent tokens to every Auto session's system prompt. Either way it needs an eval A/B rather than a code review. Could you run one alongside this PR — same task set, fragment on vs off, tool-failure retry rate and completion rate? If it moves the numbers I'll review the implementation properly. 简体中文先暂停评审——前提问题写在 #3507。实现质量不是问题:443 行 5 个文件,纯函数 resolver,准入条件收得很干净,PR 里也没有塞设计文档,边界很好。 但有一点从 diff 就能看出来,值得现在提:这个片段并没有在做 issue 要求的事。#3507 引用的是一条指令性的提示——能用 Bash 完成的就用 Bash,只有 Bash 确实做不到时才回退到专用工具。而 所以两条路通向同一个地方:如果重试循环问题真实存在,这段文字大概率推不动它;如果不存在,我们就是在每个 Auto 会话的系统提示里永久多加了一段。无论哪种,需要的是 eval 对照,而不是 code review。 能否和这个 PR 一起跑一组?同一批任务,开/关片段,看工具失败重试率和完成率。数字有变化,我就正式评审实现。 |
Closes #3507
What changed
packages/runtime/src/system-prompt/auto-tool-guidance.ts.Scope
This changes prompt guidance only. It does not change permission classification, sandbox or approval execution, Bash schema/implementation, persistence protocols, or the
headless-coding-v1contract.Verification
git diff --checkpassed.AI use
Tool: OpenAI Codex assisted with issue analysis, implementation, tests, and verification.